[Utility] Feat: add client-side session cache #888
Conversation
app/client/cli/cache/session.go
Outdated
| // SessionCache stores and retrieves sessions for application+relaychain pairs | ||
| // | ||
| // It uses a key-value store as backing storage | ||
| type SessionCache struct { |
There was a problem hiding this comment.
Thoughts on making SessionCache an interface with an internal sessionCache implementation so its easy to see & understand how to interact with it?
There was a problem hiding this comment.
Not against it at all, and I have used the pattern, i.e. define a public interface and return an internal struct as the implementation, many times. However, Accept interfaces, return structs pattern is arguably a good substitute, as it matches the standard library. Either approach works here IMO, so please advise which you consider a better fit.
There was a problem hiding this comment.
Let's do the following:
- Define a public interface
- Make the public interface conform to the
Submoduleinterface - Define an internal struct
- Have the factory function return the interface type (which is actually an initialization of the struct underneath)
There was a problem hiding this comment.
Closing this out per @h5law's suggestion here: #888 (comment)
|
@adshmh Please re-request a review when this is ready |
There was a problem hiding this comment.
Left a bump on a previous comment, and for the few improve/todo comments that have been added the only minor NIT I could add would be to create a ticket to track them and add the issue number in the comment IMPROVE(#XXX) for example.
Otherwise everything looks good! Make sure to address the linter errors and I think this will be ready to merge then
reviewpad
bot
added
large
app/client/cli/cache/session.go
Outdated
| // SessionCache stores and retrieves sessions for application+relaychain pairs | ||
| // | ||
| // It uses a key-value store as backing storage | ||
| type SessionCache struct { |
There was a problem hiding this comment.
Let's do the following:
- Define a public interface
- Make the public interface conform to the
Submoduleinterface - Define an internal struct
- Have the factory function return the interface type (which is actually an initialization of the struct underneath)
Thank you for the review. Fixed, except item 2, as conforming to the |
To be clear the @adshmh Personally as this is not a submodule I would change your |
Co-authored-by: Daniel Olshansky <[email protected]>
## Description Add a client-side cache for sessions and use it in the `servicer` command. ## Issue Fixes #791 ## Type of change Please mark the relevant option(s): - [x] New feature, functionality or library - [ ] Bug fix - [ ] Code health or cleanup - [ ] Major breaking change - [ ] Documentation - [ ] Other <!-- add details here if it a different type of change --> ## List of changes - A session cache in the client package - Use the new session cache in the servicer command ## Testing - [x] `make develop_test`; if any code changes were made - [x] `make test_e2e` on [k8s LocalNet](https://github.com/pokt-network/pocket/blob/main/build/localnet/README.md); if any code changes were made - [ ] `e2e-devnet-test` passes tests on [DevNet](https://pocketnetwork.notion.site/How-to-DevNet-ff1598f27efe44c09f34e2aa0051f0dd); if any code was changed - [ ] [Docker Compose LocalNet](https://github.com/pokt-network/pocket/blob/main/docs/development/README.md); if any major functionality was changed or introduced - [x] [k8s LocalNet](https://github.com/pokt-network/pocket/blob/main/build/localnet/README.md); if any infrastructure or configuration changes were made ## Required Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added, or updated, [`godoc` format comments](https://go.dev/blog/godoc) on touched members (see: [tip.golang.org/doc/comment](https://tip.golang.org/doc/comment)) - [x] I have tested my changes using the available tooling ### If Applicable Checklist - [ ] I have updated the corresponding README(s); local and/or global - [x] I have added tests that prove my fix is effective or that my feature works - [ ] I have added, or updated, [mermaid.js](https://mermaid-js.github.io) diagrams in the corresponding README(s) - [ ] I have added, or updated, documentation and [mermaid.js](https://mermaid-js.github.io) diagrams in `shared/docs/*` if I updated `shared/*`README(s) --------- Co-authored-by: Daniel Olshansky <[email protected]>
Description
Add a client-side cache for sessions and use it in the
servicercommand.Issue
Fixes #791
Type of change
Please mark the relevant option(s):
List of changes
Testing
make develop_test; if any code changes were mademake test_e2eon k8s LocalNet; if any code changes were madee2e-devnet-testpasses tests on DevNet; if any code was changedRequired Checklist
godocformat comments on touched members (see: tip.golang.org/doc/comment)If Applicable Checklist
shared/docs/*if I updatedshared/*README(s)